Conversation
| double yCoordinate | ||
| ) { | ||
| public LocationDto(String addressCode, String name, Point point) { | ||
| this(addressCode, name, point.getX(), point.getY()); |
There was a problem hiding this comment.
포인트의 맥락에서는 x, y가 맞는 것 같기는한데요
location의 xCoor~와 다르게 한 이유가 있나요?
There was a problem hiding this comment.
@f-lab-k
라이브러리에서 제공해주는 메소드를 쓰고있는 부분인데 location에서 동일한 형태로 맞출 때
메소드명이 구체적이지 않은 것 같아.. 우선 임의로 수정했습니다!
| private final MenuRepository menuRepository; | ||
|
|
||
| public List<Menu> findByStoreIdxAndSort(Long idx, MenuSort menuSort) { | ||
| Objects.requireNonNull(idx, "Store idx must not be null"); |
There was a problem hiding this comment.
- 현재 이렇게 예외를 발생시키면 client한테 의도한 대로 예외가 잘 도달하나요?
- 요거는 그냥 의견인데 sort가 함수명에 들어갈 필요가 있을까 싶어요
There was a problem hiding this comment.
- 음.. NPE에 대하여 클라이언트에게 의도된 메세지를 전달해주지 못하는 이슈가 있을 것 같네요..
보여주고 하는 메세지를 반환할 수 있도록 NPE관련 에러 핸들러를 구현했습니다.
추가적으로 도메인에 대한 NPE 관련 에러는 미리 만들어두고 사용하는 편일까요?
유효성 검증에 대한 에러에 대하여 반복적으로 사용하는 에러일 경우,도메인별로 에러를 정의하는지 궁금합니다!
(도메인별 NPE, 빈값 체크등, ㄷ유효성관련검사)
- 말씀해주신 오버로딩하여 동일한 역할을 하는 메소드임을 나타내는게 더 좋을 것 같네요!!
조회하는 역할만 하는 메소드를 하나 지어
매개변수만 다르게 한다면 해당 도메인을 찾는 메소드에 대하여 메소드는 하나임을 인지시킬수 있고,
다양한 매개변수를 통해 여러가지 방식을 지원한다고 나타낼 수 있을 것 같습니다!
| } | ||
|
|
||
| public List<ReserveDto> reserveDtoList(Long idx, LocalDate localDate) { | ||
| return repository.storeReserveDtoBeforeMaxDate(idx, localDate) |
There was a problem hiding this comment.
Service Layer 계층에서만 사용하고자 하는 DTO를 생성하고 있습니다.
혹시 Repository에서 Dto를 사용하는게 지양되는 부분일까요?
그렇다면 join과 관련된 행위들을 할 경우 어떻게 데이터를 가져와야 할지 궁금합니다!
| public Map<Long, StoreBusinessDto> findBusinessMap(List<Long> idxes) { | ||
| return storeBusinessHourRepository.findByStoreIdxIn(idxes) | ||
| DayType day = DayType.from(LocalDate.now()); | ||
| return storeBusinessHourRepository.findByStoreIdxIn(idxes, day) |
There was a problem hiding this comment.
LocalDate.now()가 있어서 테스트가 어려워 질 수 있겠는데용?
There was a problem hiding this comment.
제일 외부 층인 컨트롤러에서 시간을 가져올 수 있도록 기능을 수정하겠습니다!
| } | ||
|
|
||
| public StoreDetailDto storeDetailDto(Long idx) { | ||
| Store store = storeReadService.findByIdx(idx); |
There was a problem hiding this comment.
dto라는 네이밍으로 유추할 수 있다고 생각했는데, 동사로 시작해야 하는 규칙을 깬 것 같아 수정하겠습니다!
| private final StoreAmenityReadService amenityReadService; | ||
|
|
||
| public StoreListDto getStoreListDto(StoreSearchRequestDto storeSearchRequestDto) { | ||
| Stores stores = Stores.from(storeReadService.findBySearchDto(storeSearchRequestDto.toSearchDto())); |
There was a problem hiding this comment.
어디는 list를 사용하시고 아래에는 es라는 복수를 사용하시네요
일관성이 있으면 더 좋을 것 같아요
| value = """ | ||
| select | ||
| reserve_data AS date, | ||
| IF(SUM(reserved_count) > 0, true, false) AS reserve, |
There was a problem hiding this comment.
오홍...로직은 앵간하면 application에서 하는게 낫지 않으려나요??
There was a problem hiding this comment.
기존 PR에도 동일한 질문을 남겨주셔셔 수정완료했습니다!
추가적으로 해당 PR에 궁금한점 남겨놨습니다!
| @RestControllerAdvice | ||
| @RequiredArgsConstructor | ||
| @Slf4j | ||
| public class ControllerAdvice { |
There was a problem hiding this comment.
ControllerAdvice, RestControllerAdvice 두개다 있어서
구분하는게 낫지 않으려나여?
ControllerAdvice -> RestControllerAdvice로
|
|
||
| @Override | ||
| public String errorCode() { | ||
| return "BAD_REQUEST_" + errorCode; |
There was a problem hiding this comment.
얘도 bad request라면 Bad Request Error를 위에 구현해두신 것 같은데 그거 사용하면 되지 않으려나여?
# Conflicts: # src/main/java/com/yscp/catchtable/application/store/dto/StoreDto.java # src/main/java/com/yscp/catchtable/domain/store/repository/StoreBusinessHourRepository.java # src/test/java/com/yscp/catchtable/domain/reserve/repository/ReserveRepositoryTest.java
|
@f-lab-k 리뷰 해주셔서 감사합니다! |
|
2 participants
배경
Issue
#4
비고